Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

feat: add WindowTitle setting #507

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

feat: add WindowTitle setting #507

wants to merge 1 commit into from

Conversation

nixpig
Copy link

@nixpig nixpig commented Jul 24, 2024

Per issue: #436

Set WindowTitle "Live in the Terminal"

image

@nixpig nixpig requested a review from maaslalani as a code owner July 24, 2024 04:40
@nixpig nixpig mentioned this pull request Jul 24, 2024
Merged
@Delta456
Copy link
Contributor

Wow, this looks awesome. Would totally vouch for this to be implemented!

caarlos0
caarlos0 approved these changes Oct 9, 2024
View reviewed changes
Copy link
Member

@caarlos0 caarlos0 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this looks great, thank you!

i do think the font of the title could be a bit smaller though... 🤔

@meowgorithm
Copy link
Member

meowgorithm commented Oct 9, 2024 •
edited
Loading

Agreed, this one isn’t quite there yet in terms of typography. Can we bump it down a hair and ensure that it only uses exactly the same typeface as the terminal? By default that should be JetBrains Mono (via embedding).

It’s very important for the integrity of the project that we get this one right before merging.

Also, we’ll need to update the README for this as well.

@nixpig
Copy link
Author

nixpig commented Oct 13, 2024

Cool, thanks for the feedback. I'll get those changes implemented.

@nixpig
Copy link
Author

nixpig commented Oct 18, 2024

Just wanted to confirm the expected behaviour for a few scenarios:

Scenario Fonts available on system Terminal font Title font
No font specified via Set FontFamily.
  • JetBrains Mono available on system.
 JetBrains Mono from system. JetBrains Mono from system seems reasonable, but could load from embedding.
No font specified via Set FontFamily.
  • JetBrains Mono not available on system.
  • Next available on system from defaultFontFamilies is Hack.
    		 Hack from system. JetBrains Mono from embedding or Hack from system?
    No font specified via Set FontFamily.
    • No fancy fonts from defaultFontFamilies available on system.
      		 Fallback to bare monospace. JetBrains Mono from embedding or fallback to bare monospace?
      Set FontFamily "JetBrains Mono"
      • JetBrains Mono available on system.
      		 JetBrains Mono from system. JetBrains mono from system seems reasonable, but could load from embedding.
      Set FontFamily "JetBrains Mono".
      • JetBrains Mono not available on system.
      		 Fallback to bare monospace. JetBrains mono from embedding or bare monospace?
      Set FontFamily "Hack".
      • Hack available on system.
      		 Hack from system. Hack from system.
      Set FontFamily "Hack".
      • Hack not available on system.
      		 Fallback to bare monospace. JetBrains Mono from embedding or fallback to bare monospace?

      Any other scenarios not covered above, please add.

      Also, should the font file to embed for JetBrains Mono be included in the repo, or add a script/make target to download it from JetBrains at build time, or some other approach?

      @bashbunni bashbunni added the enhancement New feature or request label Oct 22, 2024
      Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
      Reviewers

      @caarlos0 caarlos0 caarlos0 approved these changes

      @maaslalani maaslalani Awaiting requested review from maaslalani

      Assignees
      No one assigned
      Labels
      enhancement New feature or request
      Projects
      None yet
      Milestone
      No milestone
      Development

      Successfully merging this pull request may close these issues.
      5 participants
      @nixpig @Delta456 @meowgorithm @caarlos0 @bashbunni